First draft of new climatology config #1356
Conversation
|
Two tests fail due to different mean obs than expected with new climatology. |
I remember climatology previously was hard coded, in the pyaerocom const to be defined as the mean between 2005- 2015, so if you use different period it might cause the test failing. The calc_climatology function in helpers.py will set the year of the climatology unless defined (the |
pyaerocom/climatology_config.py
Outdated
|
|
||
| resample_how: str = const.CLIM_RESAMPLE_HOW | ||
| freq: str = const.CLIM_FREQ | ||
| mincount: dict = const.CLIM_MIN_COUNT |
There was a problem hiding this comment.
I think set_year should also be an attribute here, or if you have a better solution?
| stop=use_climatology_ref.stop, | ||
| clim_mincount=use_climatology_ref.mincount, | ||
| resample_how=use_climatology_ref.resample_how, | ||
| clim_freq=use_climatology_ref.freq, |
There was a problem hiding this comment.
Same here set_year should be set
| if isinstance(use_climatology_ref, ClimatologyConfig): # pragma: no cover | ||
| col_freq = "monthly" # use_climatology_ref.freq | ||
| obs_start = use_climatology_ref.start | ||
| obs_stop = use_climatology_ref.stop |
There was a problem hiding this comment.
This seems like an old hack or something(?). Default const.CLIM_FREQ is daily, and is thus used everywhere climatology is calculated. But the col freq is set as monthly if climatology is used. Why?
And changing col_freq in the colocate_gridded_ungridded, is that smart? Then the value from colocation config is no longer immutable(?)
There was a problem hiding this comment.
Like we talked about, we should validate this asap, in the colocation_setup, so that no data is read before this change.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main-dev #1356 +/- ##
============================================
- Coverage 78.97% 78.49% -0.48%
============================================
Files 136 137 +1
Lines 20818 20891 +73
============================================
- Hits 16441 16399 -42
- Misses 4377 4492 +115
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| How to resample the climatology. Must be mean or median. | ||
| freq : str, optional | ||
| Which frequency the climatology should have | ||
| mincount : dict, optional |
There was a problem hiding this comment.
I'd suggest min_count. Also maybe worth building a field_validator for this? Or possibly a child class of TypedDict
There was a problem hiding this comment.
Should this module live in pyaerocom/aeroval? I don't really see this being used in the core API
There was a problem hiding this comment.
As of now, it is passed though the colocator, meaning that it is part of the core. If this is moved from the core to aeroval, then some more work is needed
There was a problem hiding this comment.
If that's the case then, no, it should stay here in the core. We don't want to have pyaerocom depend on pyaeroval after all.
| if isinstance(use_climatology_ref, ClimatologyConfig): # pragma: no cover | ||
| col_freq = "monthly" # use_climatology_ref.freq | ||
| obs_start = use_climatology_ref.start | ||
| obs_stop = use_climatology_ref.stop |
There was a problem hiding this comment.
Like we talked about, we should validate this asap, in the colocation_setup, so that no data is read before this change.
Change Summary
Adds new way of defining how the climatology is defined. Instead of just a bool to turn climatology off and on, a climatology config can now be passed into the colocation setup.
This is a reimplementation of @Ovewh 's PR #1135. As pyaercom has changed as much as it has, it was easier to do everything from scratch.
Related issue number
Aims to fix #1125
Checklist